Repository navigation
fix(LayoutPanel): a panel at width 0 takes no space and shows none of its children - #9992
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
meta-cla
Bot
added
the
CLA Signed
|
AI review status for this pull request.
|
github-actions
Bot
added
the
needs:design-review
github-actions
Bot
added
the
needs:spec-owner-review
|
/approve-spec 4dc971d |
PR Analysis Report🔎 Preview ·
|
| Component | Size | LoC | Visual · A11y · RTL |
|---|---|---|---|
| LayoutFooter | 5.9 kB (0) · 1.6 kB CSS (0) | — | ✓ · ✓ · ✗ |
| LayoutHeader | 6.0 kB (0) · 1.6 kB CSS (0) | — | ✓ · ✓ · ✗ |
| LayoutPanel | 20.5 kB (+34 B) · 4.9 kB CSS (0) | — | ✓ · ✓ · ✓ |
3 more components
| Component | Size | LoC | Visual · A11y · RTL |
|---|---|---|---|
| AppShell | 76.3 kB (+15 B) · 12.2 kB CSS (0) | — | — · — · — |
| Layout | 6.8 kB (0) · 2.3 kB CSS (0) | — | ✓ · ✓ · ✓ |
| LayoutContent | 17.4 kB (0) · 1.7 kB CSS (0) | — | ✓ · ✓ · ✓ |
Package size: @astryxdesign/core 6.56 MB packed (+208 B); no new dependencies.
API changes: none in this pull request
Committed API report behind this build: cli 9 rows, core 4 rows; run pnpm api:report.
Touched components checked; add ci:run-visual, ci:run-a11y or ci:run-rtl to run the full suites.
Generated by PR Enrichment workflow | View full report
github-actions
Bot
removed
the
needs:spec-owner-review
![astracat-bot[bot]](https://avatars.githubusercontent.com/in/4796231?s=60&v=4){kind=small}
There was a problem hiding this comment.
Approved pending CI: CI has not run on this head yet. Because this is a draft from a fork, the next steps are to mark it ready for review and for a maintainer to add the ci:run label so CI runs.
No blocking findings: a panel at numeric width 0 now measures 0px (1px with a divider), the new browser spec fails without the fix and passes with it, and panels at other widths render unchanged.
Optional follow-ups:
- In
collapsedPanel.interaction.browser.spec.ts, the start panel's child-visibility check also passes without the fix, because the ResizeHandle's grab zone covers the squeezed heading andelementFromPointnever reaches it; counting child textRangerects that intersect the panel's padding box would make it fail before the fix. - The comment at
LayoutPanel.tsx:240-241repeats whatisZeroWidthand the style comment above say;// A collapsed region sizes its panel to 0.keeps the one fact the code cannot show.
[Automated review]
| expect(reading.childCount, testId).toBeGreaterThan(0); | ||
| expect(reading.width, testId).toBe(divider); | ||
| expect(reading.paddingInline, testId).toBe(0); | ||
| expect(reading.shownChildPoints, testId).toBe(0); |
There was a problem hiding this comment.
Non-blocking: for collapsed-start this assert also passes without the fix. On the base the heading paints at x 49-58, under the ResizeHandle's 16px grab zone (x 41-57), so elementFromPoint returns the zone and the count is 0 while the letters show. Only the width and padding asserts fail there. Counting child text Range rects that intersect the panel's padding box would make this check fail before the fix for both panels.
There was a problem hiding this comment.
Fixed in 266896a: the check now counts the children's laid-out rects, text runs included, inside the panel's padding box, so it reads geometry instead of hit tests. On main it fails for both panels: 27 (start) and 7 (end) rects show there, in Chromium and WebKit.
| // A panel at width 0, such as one a collapsed region sizes, takes no space | ||
| // and shows none of its children. |
There was a problem hiding this comment.
Non-blocking: this repeats what isZeroWidth and the zeroWidth style comment already say. Keep only the fact the code cannot show:
// A collapsed region sizes its panel to 0.There was a problem hiding this comment.
Fixed in 266896a: the comment is now "A collapsed region sizes its panel to 0."
4dc971d to
266896a
Compare
There was a problem hiding this comment.
Approved pending CI: CI has not run on this head yet. Because this is a draft from a fork, the next steps are to mark it ready for review and for a maintainer to add the ci:run label so CI runs.
No blocking findings: a panel at numeric width 0 now measures 0px (1px with a divider), the reworked browser check fails without the fix for both panels and passes with it, and panels at other widths render unchanged. Both earlier suggestions, the hit-test check and the restating comment, are addressed.
[Automated review]
cixzhang
added
the
ci:run
266896a to
063df65
Compare
github-actions
Bot
added
the
needs:spec-owner-review
|
/approve-spec 063df65 |
github-actions
Bot
removed
the
needs:spec-owner-review
There was a problem hiding this comment.
Changes requested: the width-0 fix works (a collapsed or width={0} panel now measures 0px, 1px with a divider, with nothing showing, and the new browser spec fails without it), but one blocking issue remains.
- At width 0,
LayoutPanel.tsxoverrides the role and label fromuseScrollRegionPropsinstead of passing itisScrollable: isScrollable && !isZeroWidth, which copies the hook's not-scrolling branch and keeps its overflow observer attached to a panel that now clips, against LayoutPanel PR1 ("no ... observer ... except the overflow observation of AR3 while it may scroll").
With that one-call change, rendering region.role and region['aria-label'] as LayoutContent does, both new browser cases still pass in Chromium. Optional: the changeset's clause about a collapsed panel staying a group named "Content" describes behavior from another pending changeset that no release has shipped. CI has not run on this head yet; a maintainer adds the ci:run label once it is ready for review.
[Automated review]
| // At width 0 the panel clips its content, so it does not scroll. | ||
| role={isZeroWidth ? role : region.role} | ||
| aria-label={isZeroWidth ? label : region['aria-label']} |
There was a problem hiding this comment.
Blocking: these ternaries copy useScrollRegionProps's own not-scrolling branch, while the hook (still told isScrollable on line 251) keeps its overflow observer on a panel that now clips, against LayoutPanel PR1. One call covers both:
const region = useScrollRegionProps({
isScrollable: isScrollable && !isZeroWidth,
role,
label,
});Then render role={region.role} and aria-label={region['aria-label']}, as LayoutContent does, and drop this comment. With that change both cases in collapsedPanel.interaction.browser.spec.ts still pass in Chromium.
There was a problem hiding this comment.
Fixed in 82d2537, with the suggested call: the hook is told isScrollable && !isZeroWidth, so it stops observing a panel at width 0, and the panel renders region.role and region['aria-label'] as LayoutContent does. The changeset is back to the visual-only text.
…f its children
A browser fixture story renders a start LayoutPanel given a collapsed
region's props, with no divider, and an end panel at `width={0}`, with
a divider. The spec reads each panel in Chromium and WebKit. First, its
children must have laid-out boxes or text runs. Then its border box must
be 0, or the divider's 1px, its inline padding 0, and none of those
child rects may reach into its padding box, where its clip shows them.
The check reads geometry, because the handle's grab zone can cover the
panel. On main each panel keeps 32px of padding and its text shows
inside it, so this fails until the next commit.
A second fixture has a scrolling end panel whose content overflows,
with a button that collapses it to width 0 and opens it again. While it
is open, the panel must be the named group of the scrolling-region rule.
At width 0 it must not be, and it must be the group again once it opens.
The overflow observer cannot measure a box of width 0, so it keeps its
last answer and the panel stays a group. This also fails until the next
commit.
… its children A LayoutPanel at width 0, such as one a collapsed region sizes, drops its inline padding and the collapse margin it takes without a divider, and clips. Its border box was the padding's 32px, and the children laid out in a content box of no width showed squeezed in that padding. component:LayoutPanel FR1 has `width`, or the `resizable` size, set the inline size, so the panel now renders the 0 it is given. A panel at width 0 clips its content, so it does not scroll, and the scroll-region hook is told so: it stops observing the panel (PR1), and the panel takes only the caller's `role` and `label` (AR3). Before, the overflow observer could not measure a box of width 0, so a scrolling panel kept the group role and its name while collapsed.
… browser checks The FR1 and ORD1 verification row names the new browser spec for a panel at width 0, and its states add zero width and a collapsed region. The AR3 row names the same spec, adds a scrolling panel collapsed to width 0 to its states, and fails a group role kept at width 0. The cells keep their widths, so no other row re-pads. A `[fix]` changeset describes the change.
063df65 to
82d2537
Compare
github-actions
Bot
added
the
needs:spec-owner-review
|
/approve-spec 82d2537 |
github-actions
Bot
removed
the
needs:spec-owner-review
There was a problem hiding this comment.
Approved pending CI: CI has not run on this head yet. Because this is a draft from a fork, the next steps are to mark it ready for review and for a maintainer to add the ci:run label so CI runs.
No blocking findings: a LayoutPanel at numeric width 0 now measures 0px (1px with a divider) with none of its children showing, a scrolling one stops being a named group at that width and becomes one again when it opens, and the new browser spec fails without the fix and passes with it. The earlier issue is resolved: the panel now tells useScrollRegionProps it does not scroll at width 0 and renders the hook's role and label, so the overflow observer stops there.
[Automated review]
There was a problem hiding this comment.
Approved. A LayoutPanel at numeric width 0 (width={0}, or a collapsed useResizable region kept mounted) now measures 0px, or 1px with a divider, with none of its children showing; a scrolling one is no longer a group named "Content" at that width and becomes one again when it opens.
The new browser spec fails without the fix and passes with it in Chromium and WebKit, panels at other widths render unchanged, and CI passes on this head. No blocking findings.
[Automated review]
…#10077) fix(a11y): give the collapsing scroll-panel fixture focusable content The full accessibility audit reports `scrollable-region-focusable` on `core-resizable--collapsing-scroll-panel-fixture`, the fixture #9992 added: its open LayoutPanel scrolls 40 lines with nothing focusable in them. The lines are buttons now, as #10019 did for the scroll-region name stories. A keyboard reaches the scrolling content without a tab stop on the panel. The panel still overflows while it is open, so LayoutPanel's width-0 browser check reads the same. No product change and no baseline entry.
… covers (#10081) ci(a11y): the touched check audits every changed story no other audit covers The touched check audited changed stories only when no component owned them, so a story added to an untouched component's file escaped: #9992 added core-resizable--collapsing-scroll-panel-fixture to Resizable's stories while touching LayoutPanel, and the scrollable-region-focusable violation surfaced only at main's release-check. The story audit now takes every story of a changed story file except those the touched components' audit already runs (--covered-components, the same filters), new violations only. A Lab-only pull request keeps its own lane (FR26). AST-030 FR12 says so.
User impact
A LayoutPanel at width 0 (a collapsed region's panel, or
width={0}) showed a 32px strip of squeezed children. It should take no space and show nothing.Problem and solution fit
border-box) keeps it 32px wide, and children in a content box of no width overflow into that padding, outside the clip.Expected behavior and authority
component:LayoutPanelFR1: "widthsets its inline size, with numbers as pixels; aresizablevalue's size wins overwidth." At 0 it rendered 32px. AR3 makes it a group only "while the panel scrolls"; at width 0 it clips.Smallest restoration
One style for the width number 0, where the panel also tells the scroll-region hook it does not scroll, so its observer stops (PR1); it cannot measure a box of width 0, so a scrolling panel stayed a group named "Content". Other widths are unchanged; a divider keeps its 1px line. A bare
width={0}panel's controls stay in the Tab order (AR2's gap; #9300 closes it for a region's panel).Evidence
Desktop Chromium 149, 1280×800, the collapsed-panel story; before is main's code plus the story (the first commit), after is this head.
role="group".scrollRegionName.collapsedPanel.interaction.browser.spec.tsfails before the fix in Chromium and WebKit.Merge order: #9989 also edits
Resizable.stories.tsxand a neighboring verification row.Writing rubric results
Over
component:LayoutPanel: FR1's and AR3's verification rows name the browser spec and its width-0 states and failure; cells keep their widths. Owner-gated.Scope
Testing
Local checks at this head, on main 1fec9fa (#9300 merged):
CI=true pnpm lint(0 errors), check:changesets, check:knowledge, core typecheck, prettier on the changed files; afterpnpm build,pnpm api:reportshows no LayoutPanel change.collapsedPanel,grabZone,ResizeHandleandSideNavRailMotionin WebKit (25 passed, 12 skipped); the Layout, Resizable, SideNav andscrollRegionNamea11y contracts in Chromium (47 passed).collapsedPanel.interaction.browser.spec.tscases fail in Chromium and WebKit: the width-0 readings[width, padding, shown]are start[32, 32, 27]and end[33, 32, 7], and the collapsing panel stays 33px wide. With the width fix alone, the second case fails at the group role.paneFocus.test.tsx's lent-pane case, local probes (not part of the change) count 6 take-backs against 2 without the swap; the case still passes, and a re-lend broken on purpose fails it.